Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes for missing localIds #1309

Merged
merged 34 commits into from
Jan 5, 2024
Merged

Fixes for missing localIds #1309

merged 34 commits into from
Jan 5, 2024

Conversation

JPercival
Copy link
Contributor

@JPercival JPercival commented Dec 22, 2023

  • new IdObjectFactory that assigns an id to all objects created
    • refactoring to share the factory everywhere
  • new FuntionalElmVisitor to facilitate walking the ELM graph
    • tests for same
  • tests to validate that every node has a localId
  • fixes to serialization of empty collections, discovered while testing the above
  • fixes to data requirements processor that was dependent upon elements not having unique localIds.

There was a refactor to separate and reorder the phases of the compiler, which can be seen here:
https://github.com/cqframework/clinical_quality_language/blob/bug-localId-tests/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/CqlCompiler.java#L208-L241

Resolves #1295, #1279, #1131, #704, #697

Copy link

Formatting check succeeded!

Copy link

codecov bot commented Dec 22, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (cc869ca) 62.70% compared to head (f9b65d2) 62.79%.

Files Patch % Lines
.../java/org/cqframework/cql/cql2elm/elm/ElmEdit.java 80.00% 2 Missing and 2 partials ⚠️
...va/org/cqframework/cql/cql2elm/LibraryBuilder.java 90.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1309      +/-   ##
============================================
+ Coverage     62.70%   62.79%   +0.09%     
- Complexity     6079     6090      +11     
============================================
  Files           466      468       +2     
  Lines         25308    25322      +14     
  Branches       4711     4714       +3     
============================================
+ Hits          15869    15901      +32     
+ Misses         7288     7280       -8     
+ Partials       2151     2141      -10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JPercival JPercival merged commit 6477b7f into master Jan 5, 2024
4 checks passed
@JPercival JPercival deleted the bug-localId-tests branch January 5, 2024 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Elm Translator: Missing localId for choice type properties
3 participants